Add the ability to opt into logging on a per-endpoint basis #103
Conversation
Also, expose conditionals that will by default opt all endpoints into being logged. Note one change, responses can now be logged without an actual request being logged.
request_logging/middleware.py
Outdated
| if because is not None: | ||
| return self._skip_logging_request(request, because) | ||
| should_log_route = self._should_log_route(request) | ||
| if not should_log_route.log_route: |
There was a problem hiding this comment.
I realize this mirrors the old code, but why the nested ifs instead of one and? Also, it seems like we should dump out in either case so it should be an or (and maybe we provide an empty string if there's no skip_reason).
request_logging/middleware.py
Outdated
| if should_log_response: | ||
| # Either the response is opted-in to logging by default or we've | ||
| # conditionally selected the response to be logged. Regardless, log it! | ||
| if 400 <= response.status_code < 500: |
There was a problem hiding this comment.
Please break this out into its own function,
By default, do not log a response if a request is skipped from being logged as well.
|
@tclancy apologies for this taking so long to get back to, I've been in the middle of a move and unable to get to this. I think I've addressed your feedback though (and hoping that tests all pass of course) |
| def _should_log_route(self, request): | ||
| func = self._get_api_func(request) | ||
| if func in OPT_INTO_LOGGING_FUNCS: | ||
| return ShouldLogRoute(log_route=True, skip_reason=None, silent=False) | ||
|
|
||
| if func in NO_LOGGING_FUNCS: | ||
| no_logging = NO_LOGGING_FUNCS[func] | ||
| return ShouldLogRoute(log_route=False, skip_reason=no_logging.message, silent=no_logging.silent) | ||
|
|
||
| return ShouldLogRoute(log_route=self.logging_opt_in_defaults[REQUEST](request), skip_reason=None, silent=False) |
There was a problem hiding this comment.
I couldn't really come up with a better way to handle the silent case than propagating a flag forward. It definitely feels a little leaky but it works with current behavior
|
Closing this because it's super old and I don't have the time to update this 😢 |
Also, expose conditionals that will by default opt all endpoints into
being logged.
Note one change, responses can now be logged without an actual request
being logged.
This may be a little more opinionated than needed but I personally prefer not using an attribute on a view method as a flag on whether or not to log the method. For newer versions of Python dictionaries are fairly compact so I think storing methods that we don't want logged / want to opt into logging is a reasonable thing to do (and I think it makes the code a lot cleaner)